Skip to content

PiPNN 5/6: add dedicated benchmark pipelines - #1294

Open
weiyaoluo (SeliMeli) wants to merge 19 commits into
pipnn-stack/04-integrationfrom
pipnn-stack/05-benchmark
Open

PiPNN 5/6: add dedicated benchmark pipelines#1294
weiyaoluo (SeliMeli) wants to merge 19 commits into
pipnn-stack/04-integrationfrom
pipnn-stack/05-benchmark

Conversation

@SeliMeli

@SeliMeli weiyaoluo (SeliMeli) commented Jul 29, 2026

Copy link
Copy Markdown

Purpose

This PR adds dedicated benchmark routes for PiPNN graph and disk builds.

PiPNN builds adjacency in one batch. The benchmark must not use the incremental Vamana insertion path.

Main changes

  • Graph inputs accept BuildAlgorithm::PiPNN.
  • pipnn_build creates the requested Rayon pool and calls the PiPNN core.
  • The graph route installs vectors, adjacency rows, and frozen start points after the batch build.
  • The disk route calls the production builder from PiPNN 4/6: integrate disk build pipeline #1291.
  • Feature-disabled PiPNN requests fail with an error.
  • Dynamic graph jobs reject the batch-only algorithm.
  • Example JSON files cover graph and disk routes.

BuildStats reports one batch duration. It does not report synthetic per-insert latency.

Review order

  1. Review PiPNN input parsing in inputs/graph_index.rs and inputs/disk.rs.
  2. Review index/build.rs::pipnn_build in execution order.
  3. Review route selection in index/benchmarks.rs.
  4. Review the disk route into diskann-disk.
  5. Review example inputs and CLI tests.

Validation

  • Tests check the requested start strategy and frozen adjacency.
  • Tests check that batch builds omit insert percentiles.
  • Tests check feature-disabled and dynamic-job errors.
  • CLI tests cover graph and disk dispatch.
  • All-target Clippy passes.

Stack

Stack 5/6. Depends on #1291. #1295 adds optional HashPrune merging.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The pipnn feature definition likely does not enable the optional diskann-disk dependency (breaking --features pipnn builds) and should be corrected before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds PiPNN-specific benchmark entry points and fixtures so benchmarks can build PiPNN graphs (in-memory) and PiPNN disk indexes through the production disk builder, with integration tests validating the selected algorithm and basic results shape.

Changes:

  • Add a dedicated in-memory PiPNN graph build pipeline in diskann-benchmark and route graph-index benchmarks to it when requested.
  • Extend disk-index benchmark input/configuration to support selecting BuildAlgorithm (Vamana vs PiPNN), and pass PiPNN through the production disk build pipeline.
  • Add PiPNN benchmark example JSONs and integration tests gated on the pipnn feature.
File summaries
File Description
diskann-benchmark/src/main.rs Extends CLI integration tests to cover PiPNN graph/disk benchmark examples.
diskann-benchmark/src/inputs/graph_index.rs Adds optional PiPNN build algorithm selection for graph-index builds (feature-gated).
diskann-benchmark/src/inputs/disk.rs Adds alpha, makes quantization optional, and adds build-algorithm selection/validation for disk-index builds.
diskann-benchmark/src/index/build.rs Implements the dedicated PiPNN in-memory build pipeline and a unit test for start strategy handling.
diskann-benchmark/src/index/benchmarks.rs Dispatches graph-index builds to PiPNN vs incremental insertion based on requested algorithm.
diskann-benchmark/src/disk_index/build.rs Plumbs BuildAlgorithm into disk index build parameters (Vamana vs PiPNN).
diskann-benchmark/example/pipnn-graph-index.json New example config exercising PiPNN graph-index build + TopK search.
diskann-benchmark/example/pipnn-disk-index.json New example config exercising PiPNN disk-index build + search.
diskann-benchmark/Cargo.toml Adds the pipnn feature and optional dependency on diskann-pipnn.
Cargo.lock Adds diskann-pipnn to the workspace lockfile dependency graph.
Review details
  • Files reviewed: 9/10 changed files
  • Comments generated: 2
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread diskann-benchmark/Cargo.toml Outdated
Comment thread diskann-benchmark/src/index/build.rs
Copilot AI review requested due to automatic review settings July 30, 2026 13:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The PiPNN benchmark path has at least one confirmed config/behavior mismatch (silently ignoring multi_insert) and an avoidable performance issue in start-point source mapping that can skew benchmark timings.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (2)

diskann-benchmark/src/index/build.rs:160

  • The PiPNN start-point source selection scans the dataset twice when an exact byte match is not found (position then min_by). For large start_point_strategy sample counts this adds avoidable O(2*N) work per start vector and skews the benchmark timing. Consider doing a single pass that checks for an exact match and otherwise tracks the best (minimum) distance as you iterate once.
    let start_sources = start_points
        .row_iter()
        .map(|start| {
            let bytes: &[u8] = bytemuck::cast_slice(start);
            data.row_iter()

diskann-benchmark/src/index/benchmarks.rs:240

  • When build_algorithm is set to PiPNN, the benchmark bypasses the incremental builder and ignores multi_insert if it was provided in the input. This can silently mislead users into thinking multi-insert settings are applied to PiPNN builds. It would be safer to reject multi_insert for PiPNN with a clear error.
                let result = match build.build_algorithm() {
                    diskann_disk::BuildAlgorithm::PiPNN(parameters) => {
                        let data =
                            Arc::new(datafiles::load_dataset(datafiles::BinFile(build.data()))?);
                        build::pipnn_build(data, build, parameters)
  • Files reviewed: 9/10 changed files
  • Comments generated: 0 new
  • Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings July 31, 2026 04:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The pipnn feature configuration and PiPNN dispatch path have correctness issues that can cause build failures and silently ignored user configuration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

diskann-benchmark/Cargo.toml:88

  • The pipnn feature enables diskann-disk/pipnn but does not enable the optional diskann-disk dependency itself. Since the benchmark code behind cfg(feature = "pipnn") references diskann_disk::BuildAlgorithm (even outside the disk-index feature), building with --features pipnn can fail due to diskann-disk not being activated.
# Enable PiPNN graph construction.
pipnn = ["dep:diskann-pipnn", "diskann-disk/pipnn"]

diskann-benchmark/src/index/benchmarks.rs:241

  • When build_algorithm selects PiPNN, this path bypasses run_build/single_or_multi_insert, so any multi_insert configuration in the input is silently ignored. That can mislead users who expect batched insertion settings to take effect; it should be rejected explicitly for PiPNN builds.
                    diskann_disk::BuildAlgorithm::PiPNN(parameters) => {
                        let data =
                            Arc::new(datafiles::load_dataset(datafiles::BinFile(build.data()))?);
                        build::pipnn_build(data, build, parameters)
                    }
  • Files reviewed: 9/10 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings August 3, 2026 02:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Not ready to approve

The PiPNN benchmark build path currently ignores multi_insert settings without rejecting them, which can lead to silently-misconfigured benchmark runs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

diskann-benchmark/src/index/build.rs:154

  • When build_algorithm selects PiPNN, the multi_insert setting from IndexBuild is silently ignored (PiPNN always runs the one-shot batch build). This can lead to confusing configs where the user thinks multi-insert is active but it has no effect; it should be rejected explicitly for PiPNN builds.
    use anyhow::Context;

    let npoints = data.nrows();
    let dimensions = data.ncols();
    let metric = input.distance().into();
    let graph = input.try_as_config()?.build()?;
  • Files reviewed: 9/10 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings August 3, 2026 10:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new pipnn feature configuration likely does not reliably enable the optional diskann-disk dependency, and there are also concrete maintainability/performance fixes needed in the new PiPNN path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

diskann-benchmark/Cargo.toml:87

  • pipnn enables diskann-disk/pipnn but does not explicitly enable the optional diskann-disk dependency. Since the pipnn-gated Rust code imports diskann_disk::*, cargo build/test --features pipnn can fail if the optional dep is not activated.
# Enable PiPNN graph construction.
pipnn = ["diskann/pipnn", "diskann-disk/pipnn"]

diskann-benchmark/src/index/build.rs:197

  • The min_by comparator recomputes evaluate_similarity(start, row) for both sides on every comparison, roughly doubling the amount of distance work when mapping synthetic start vectors to their nearest real row. You can compute the score once per row and then min_by the cached scores to cut this overhead ~2x.
                        .min_by(|(_, left), (_, right)| {
                            distance
                                .evaluate_similarity(start, left)
                                .total_cmp(&distance.evaluate_similarity(start, right))
                        })

diskann-benchmark/src/inputs/graph_index.rs:753

  • StartPointStrategy::count() is already non-zero for every variant (it uses NonZeroUsize for sampled strategies and returns 1 for the others), so unwrap_or(NonZeroUsize::MIN) can silently mask an invariant break. Using expect here both documents the invariant and avoids silently changing behavior if a zero-count strategy is ever added.
        let frozen_points =
            NonZeroUsize::new(self.start_point_strategy.count()).unwrap_or(NonZeroUsize::MIN);

  • Files reviewed: 9/9 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Copilot AI review requested due to automatic review settings August 5, 2026 12:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The pipnn feature currently doesn’t enable the optional diskann-disk dependency, so --features pipnn will fail to compile where diskann_disk::* is referenced.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (4)

diskann-benchmark/Cargo.toml:88

  • The pipnn feature enables diskann-disk/pipnn but does not enable the optional diskann-disk dependency itself. With --features pipnn (without disk-index), cfg(feature = "pipnn") code in this crate references diskann_disk::* and will fail to compile because diskann-disk is still disabled.
# Enable PiPNN graph construction.
pipnn = ["diskann/pipnn", "diskann-disk/pipnn"]

diskann-benchmark/src/index/build.rs:188

  • start_sources currently scans the full dataset twice for every start vector (first position, then a second full pass for min_by when the vector isn't found). For start strategies with many start points, this becomes a large, avoidable O(2·N·S) pass over the dataset.
    let start_sources = start_points
        .row_iter()
        .map(|start| {
            let bytes: &[u8] = bytemuck::cast_slice(start);
            data.row_iter()

diskann-benchmark/src/inputs/graph_index.rs:753

  • StartPointStrategy::count() is always non-zero today (it returns 1 for single-start variants and uses NonZeroUsize for multi-start variants). Falling back to NonZeroUsize::MIN would silently mask any future regression to 0 and can create a frozen-slot/start-vector count mismatch later in the pipeline.
        let frozen_points =
            NonZeroUsize::new(self.start_point_strategy.count()).unwrap_or(NonZeroUsize::MIN);

diskann-benchmark/src/index/build.rs:206

  • This comment says the frozen start slot "carries the chosen source vector", but DefaultProvider::set_start_points stores the start vector into the frozen slot (which can be synthetic for strategies like Medoid/RandomVectors). The source row ID is only used to choose which real row's adjacency to mirror into the slot.
    // A frozen start slot carries the chosen source vector, so expanding it
    // must expose exactly that real source's outgoing row. Prepending the source
    // ID would consume one degree slot and discard a graph edge, changing every
    // search from the graph produced by the core builder.
  • Files reviewed: 9/9 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 71.76471% with 24 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (pipnn-stack/04-integration@6706615). Learn more about missing BASE report.

Files with missing lines Patch % Lines
diskann-benchmark/src/inputs/disk.rs 0.00% 12 Missing ⚠️
diskann-benchmark/src/inputs/graph_index.rs 85.71% 5 Missing ⚠️
diskann-benchmark/src/index/streaming/stats.rs 55.55% 4 Missing ⚠️
diskann-benchmark/src/index/benchmarks.rs 90.90% 2 Missing ⚠️
diskann-benchmark/src/index/build.rs 80.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (71.76%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@                      Coverage Diff                      @@
##             pipnn-stack/04-integration    #1294   +/-   ##
=============================================================
  Coverage                              ?   91.31%           
=============================================================
  Files                                 ?      518           
  Lines                                 ?    98756           
  Branches                              ?        0           
=============================================================
  Hits                                  ?    90178           
  Misses                                ?     8578           
  Partials                              ?        0           
Flag Coverage Δ
miri 91.31% <71.76%> (?)
unittests 90.99% <71.76%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
diskann-benchmark/src/main.rs 92.07% <100.00%> (ø)
diskann-benchmark/src/index/build.rs 84.84% <80.00%> (ø)
diskann-benchmark/src/index/benchmarks.rs 71.05% <90.90%> (ø)
diskann-benchmark/src/index/streaming/stats.rs 94.81% <55.55%> (ø)
diskann-benchmark/src/inputs/graph_index.rs 54.30% <85.71%> (ø)
diskann-benchmark/src/inputs/disk.rs 1.38% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants